MDEV-38412 System tablespace fails to shrink due to legacy tables#4884
MDEV-38412 System tablespace fails to shrink due to legacy tables#4884Thirunarayanan wants to merge 1 commit into11.4from
Conversation
|
|
| DBUG_RETURN(2); | ||
| } | ||
|
|
||
| static bool should_drop_garbage_table(const rec_t* rec, ulint len) |
There was a problem hiding this comment.
This is missing noexcept, and it’d be better to use size_t instead of the alias ulint. There is no comment explaining the return value and the parameters.
storage/innobase/srv/srv0start.cc
Outdated
| @param[in] rec SYS_TABLES record containing the table name | ||
| @param[in] len length of the table name | ||
| @return true if the table should be dropped, false otherwise */ | ||
| static bool should_drop_unknown_table(const rec_t* rec, ulint len) |
There was a problem hiding this comment.
Missing noexcept. A space around the * is misplaced. Please, no [in] in new code. The @return comment should not mention individual return values; we have @retval for that.
storage/innobase/srv/srv0start.cc
Outdated
| const byte *field= rec_get_nth_field_old(rec, DICT_FLD__SYS_TABLES__ID, &id_len); | ||
| if (dict_sys.is_sys_table(mach_read_from_8(field))) | ||
| return false; |
There was a problem hiding this comment.
The if expression is missing id_len != 8 ||.
storage/innobase/srv/srv0start.cc
Outdated
| if (dict_table_t *table= dict_sys.load_table( | ||
| {reinterpret_cast<const char*>(pcur.old_rec), len}, | ||
| DICT_ERR_IGNORE_DROP)) |
There was a problem hiding this comment.
What if we have some entries in SYS_INDEXES but the table cannot be loaded because the entries in SYS_TABLES, SYS_COLUMNS, SYS_FIELDS are incorrect? In that case, we would fail to drop the table.
Do we really have to load a table definition into the cache? Would it suffice to unconditionally execute some simpler SQL to delete the corresponding entries from SYS_TABLES, SYS_INDEXES, SYS_FIELDS, SYS_COLUMNS, during a slow shutdown when :autoshrinkis enabled? What really matters here is a call todict_drop_index_tree(). That could be executed by row_purge_remove_clust_if_poss_low()` as part of the slow shutdown.
The basic algorithm would be like this:
- Collect the table ID from
SYS_TABLESand report the table names. (Check forSYS_TABLES.SPACE=0directly from each record.) - For each table ID, execute the SQL to
DELETE FROM SYS_… WHERE SPACE=0 AND TABLE_ID=:id. - Let the purge run into completion. It will take care of invoking
dict_drop_index_tree(), and it’s already checking forSYS_INDEXES.PAGE=FIL_NULLthere.
storage/innobase/srv/srv0start.cc
Outdated
| if (err == DB_SUCCESS) | ||
| { | ||
| trx->commit(); | ||
| ut_ad(deleted.empty()); |
There was a problem hiding this comment.
deleted.empty() should hold irrespective of the err value.
storage/innobase/srv/srv0start.cc
Outdated
| for (pfs_os_file_t d : deleted) | ||
| os_file_close(d); |
There was a problem hiding this comment.
There should be no handles of deleted files to close, because these tables are located in the system tablespace (fil_system.sys_space), which will not be deleted.
storage/innobase/srv/srv0start.cc
Outdated
| sql_print_information("InnoDB: Dropping the unknown table %.*s", | ||
| static_cast<int>(len), rec); |
There was a problem hiding this comment.
int(len) is shorter and equivalent to static_cast<int>(len).
Problem: ======= - InnoDB system tablespace fails to autoshrink when it contains legacy internal tables. These are non-user tables and internal table exist from older version. Because the current shrink logic does recognize these entries as user table, they block the defragmentation process required to reduce the tablespace size. Solution: ========= To enable successful shrinking, InnoDB has been updated to identify and remove these legacy entries during the startup: fsp_drop_legacy_tables(): A new function that scans the InnoDB system tables for entries lacking the / naming convention. It triggers the removal of these table objects from InnoDb system tables and ensures the purge thread subsequently drops any associated index trees in SYS_INDEXES. delete_from_sys_table_entries(): Function is to remove specific table IDs from internal system catalogs. fsp_system_tablespace_truncate(): If legacy tables are detected, InnoDB prioritizes their removal. To ensure data integrity and complete the shrink, a two-restart sequence may be required: 1) purge the legacy table 2) Defragment the system tablespace and shrink the system tablespace further
Thirunarayanan
force-pushed
the
MDEV-38412
branch
from
9d68cf6 to
c99072c
Compare
| bool drop_unknown= false; | ||
| err= fsp_drop_legacy_tables(drop_unknown); | ||
| if (err == DB_SUCCESS || drop_unknown == false) | ||
| err= space->defragment(); |
There was a problem hiding this comment.
Instead of using an output parameter, can we use the special return value DB_SUCCESS_LOCKED_REC to indicate that some unknown tables were dropped?
| /** Determine if a table should be dropped during system tablespace | ||
| autoshrinking. This function identifies legacy tables in the | ||
| system tablespace that should be removed. A table is considered a | ||
| legacy/unknown table if the table name does not contain '/' | ||
| (indicating it's not a proper database/table name) | ||
| @return error code or DB_SUCCESS */ | ||
| static dberr_t fsp_drop_legacy_tables(bool &drop_unknown) noexcept |
There was a problem hiding this comment.
This function as well as delete_from_sys_table_entries() would seem to logically belong to the compilation unit dict/drop.cc.
| const byte *field= nullptr; | ||
| ulint len= 0; | ||
| if (rec_get_deleted_flag(rec, 0)) | ||
| continue; |
There was a problem hiding this comment.
The assignments could be moved after the first if.
| sql_print_information("InnoDB: Found the unknown table %.*s", | ||
| (int) len, rec); |
There was a problem hiding this comment.
Please avoid C-style casts. int(len) is shorter too. The message does not say what we are going to do with the table:
sql_print_information("InnoDB: DROP TABLE %.*s", int(len), rec);I think that using the SQL syntax should make it clear even for DBAs who do not speak English.
| if (err == DB_SUCCESS) | ||
| trx->commit(); |
There was a problem hiding this comment.
Instead of assigning an output parameter drop_unknown, can we just assign err= DB_SUCCESS_LOCKED_REC here, to indicate that something was dropped?
| error= que_eval_sql( | ||
| nullptr, "PROCEDURE CREATE_DUMMY_1() IS\n" |
There was a problem hiding this comment.
The indentation is a bit off here.
| --source include/restart_mysqld.inc | ||
| let SEARCH_PATTERN=InnoDB: Dropping the unknown table; | ||
| --source include/search_pattern_in_file.inc | ||
|
|
||
| let SEARCH_PATTERN=InnoDB: Found the unknown table; | ||
| --source include/search_pattern_in_file.inc |
There was a problem hiding this comment.
Could we also look for the specific names of the garbage tables here? We could be outputting some garbage, and the test would not catch that.
Can we also check the contents of INFORMATION_SCHEMA.INNODB_SYS_TABLES and friends to show that the dummy tables appear and disappear as intended?
| sql_print_information("InnoDB: Dropping the unknown tables"); | ||
| trx_t *trx= trx_create(); | ||
| trx_start_for_ddl(trx); | ||
| dict_sys.lock(SRW_LOCK_CALL); | ||
|
|
||
| for (table_id_t table_id : unknown_tables) | ||
| { | ||
| err= delete_from_sys_table_entries(table_id, trx); |
There was a problem hiding this comment.
I think that we only need to lock dict_sys for invoking the SQL parser. There is no need to hold the latch across the transaction commit. What we are really missing here is a call to lock_sys_tables(trx) after the creation of the transaction.
| mtr.commit(); | ||
| if (err || unknown_tables.size() == 0) | ||
| return err; |
There was a problem hiding this comment.
We do not need mtr or pcur after this point. Could this be split into two functions?
3 participants
Problem:
InnoDB system tablespace autoshrink can fail when legacy tables exist in the system tablespace (space_id=0). These are typically old tables from earlier InnoDB versions that doesn't have '/' character in the table name.
Solution:
During system tablespace autoshrinking, InnoDB now proactively drops unknown/legacy tables that meet the following criteria:
drop_tables_by_filter(): Scan and drop tables by predicate
Used this function to drop the garbage table during restore
drop the unknown tables from system tablespace
Added DBUG_EXECUTE_IF("create_dummy_sys_tables") for testing the scenario.
This cleanup happens automatically before the autoshrink operation, preventing failures and allowing the system tablespace to be properly truncated.